Multiple converted file sliceorientationpatient #441
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #441 +/- ##
==========================================
+ Coverage 81.88% 81.91% +0.03%
==========================================
Files 41 41
Lines 4117 4141 +24
==========================================
+ Hits 3371 3392 +21
- Misses 746 749 +3 ☔ View full report in Codecov by Sentry. |
heudiconv/convert.py
Outdated
| @@ -611,6 +619,20 @@ def save_converted_files(res, item_dicoms, bids_options, outtype, prefix, outnam | |||
| imgtype, "_echo-%d%s" % (echo_number, imgtype) | |||
| ) | |||
| break | |||
| # Now check if this run is multi-orientation (v-nav or localizer) | |||
| if bids_meta and is_multiorient and 'acq-' not in this_prefix_basename: | |||
There was a problem hiding this comment.
I think that this condition should not check if there is _acq- here but rather below right before modifying the actual prefix -- announcing that because _acq- is there we would not be embedding multi-orientation information which would have been such as estimated.
heudiconv/convert.py
Outdated
| bids_pairs = this_prefix_basename.split('_') | ||
| ses_or_sub_idx = sum([bids_pair.split('-')[0] in ['sub','ses'] for bids_pair in bids_pairs]) | ||
| bids_pairs.insert(ses_or_sub_idx,'acq-%s'%slice_orient) | ||
| this_prefix_basename = '_'.join(bids_pairs) |
There was a problem hiding this comment.
not immediately clear to me why it is not just adding _acq- at the end and goes through all splitting and then joining. Please
- add a brief comment what is about to happen (something per sub/session?) and why
- make it more pep-8 friendly here ( add spaces after
,between list entries and function arguments)
re "at the end" -- you added this code AFTER possible addition of suffixes... feels like it should be before, and not interfere with suffixes handling (there is also condition after it which would not add any suffix if any changes to this_prefix_basename happen)
There was a problem hiding this comment.
and altogether - it might be worth moving this code block into a function (e.g. , so it could be unittested on its own and also make code not so long right here
There was a problem hiding this comment.
Agreed on the separate function following #424 example. Will clean up.
It thought that the BIDS key-value pairs are supposed to be in a specific order, and that _acq- is supposed to go after sub-*[_ses-*] .
As the filenames prefixes are already formed, I found this to be the easiest way to insert it at the proper location. For the other cases that #424 is dealing with (eg. https://github.com/nipy/heudiconv/pull/424/files#diff-a48009f9efe6cc64ffdb74d06e831296R341) , they choose to search through potential key-value pairs that are supposed to be after the inserted key-value, here it does the reverse because there would be too many tags to search for, as it is supposed to be right after sub or ses. Does it make sense?
Will add comment to make it more obvious.
There was a problem hiding this comment.
It thought that the BIDS key-value pairs are supposed to be in a specific order, and that _acq- is supposed to go after sub-[_ses-] .
that is correct! but it should go before suffix (like _bold) which comes before extension (.nii.gz).
There was a problem hiding this comment.
I used this file: https://github.com/bids-standard/pybids/blob/master/bids/layout/config/bids.json as a reference, which lists the following path.
"sub-{subject}[/ses-{session}]/{datatype<anat>|anat}/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{ceagent}][_rec-{reconstruction}]_{suffix<T1w|T2w|T1rho|T1map|T2map|T2star|FLAIR|FLASH|PDmap|PD|PDT2|inplaneT[12]|angio>}.{extension<nii|nii.gz|json>|nii.gz}"
Also in the BIDS specs:
https://bids-specification.readthedocs.io/en/stable/04-modality-specific-files/01-magnetic-resonance-imaging-data.html#anatomy-imaging-data
I am confused, does the order of key-value pairs matter?
There was a problem hiding this comment.
yes! order matters, sorry -- parts of my comment were misleading and incorrect. The ultimate order prescribed in BIDS itself in the entity table. I think we need to add a function which places a new key in the correct position based on other keys present.
What I was pointing also that it matters for those _key-value pairs to come before _{suffix<T1w...}.{extension<nii...}", thus my original comment:
re "at the end" -- you added this code AFTER possible addition of suffixes... feels like it should be before, and not interfere with suffixes handling (there is also condition after it which would not add any suffix if any changes to this_prefix_basename happen)
There was a problem hiding this comment.
I admit that I pretty much adapted the existing cases (which are refactored in #424), so I am basically following the same renaming procedure (even if in their cases they str.replace to insert the tag).
The variable name they used is this_prefix_basename it seems to be only the prefix, but I will be happy to adapt to any change to #424 if necessary.
bpinsard
force-pushed
the
multiple_converted_file_sliceorientationpatient
branch
from
6c8ea10 to
fa7d0ee
Compare
I looked at the test data and the scout is only one dicom file, would require at least 2 dicom files with differing ImageOrientationPatient, from a localizer for instance. Did you follow a specific procedure to include dicoms in the test set (anonymize, strip data... )? |
so you looked at But in the other tests we already use datalad to fetch full blown phantom dataset which would have "good" scouts: see https://github.com/nipy/heudiconv/blob/master/heudiconv/tests/test_regression.py#L28 which fetches http://datasets.datalad.org/?dir=/dbic/QA/ and uses tarball(s) from under http://datasets.datalad.org/?dir=/dbic/QA/sourcedata . eventually we should add proper fixtures and caching so we don't need to refetch them across tests and tests reruns (would be a nice contribution, if interested, but in a separate PR). I first thought that you could just add testing to the existing which has 3 dicoms and dcm2niix produces 3 nii.gz$> dcm2niix -9 -b y -o . -z y .
Chris Rorden's dcm2niiX version v1.0.20200331 (JP2:OpenJPEG) GCC8.3.0 (64-bit Linux)
Found 3 DICOM file(s)
Slices not stacked: orientation varies (vNav or localizer?) [0.991633 -0.0203033 0.127482 0.125499 0.382898 -0.915226] != [0.990337 -0.0649062 0.122559 0.138123 0.382102 -0.91374]
Convert 1 DICOM as ./__AAHead_Scout_32ch-head-coil_20161011084552_4_i00002 (162x162x1x1)
Warning: Check that 2D images are not mirrored.
Convert 1 DICOM as ./__AAHead_Scout_32ch-head-coil_20161011084552_4_i00003 (162x162x1x1)
Warning: Check that 2D images are not mirrored.
Convert 1 DICOM as ./__AAHead_Scout_32ch-head-coil_20161011084552_4_i00001 (162x162x1x1)
Warning: Check that 2D images are not mirrored.
Conversion required 0.052689 seconds (0.052680 for core code).
(dev3) 1 21811.....................................:Thu 23 Apr 2020 05:09:28 PM EDT:.
(git-annex)lena:…-phantoms[master]bids_test3-20161011/AAHead_Scout_32ch-head-coil_MPR_cor
$> ls
1.3.12.2.1107.5.2.43.66112.2016101108475682845701835.dcm
1.3.12.2.1107.5.2.43.66112.2016101108475682864701836.dcm
1.3.12.2.1107.5.2.43.66112.2016101108475682884501837.dcm
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00001.json
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00001.nii.gz
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00002.json
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00002.nii.gz
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00003.json
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00003.nii.gz
(dev3) 1 21812.....................................:Thu 23 Apr 2020 05:09:30 PM EDT:.
(git-annex)lena:…-phantoms[master]bids_test3-20161011/AAHead_Scout_32ch-head-coil_MPR_cor
$> grep Orient *json
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00001.json: "ImageOrientationPatientDICOM": [
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00002.json: "ImageOrientationPatientDICOM": [
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00003.json: "ImageOrientationPatientDICOM": [
(dev3) 1 21813.....................................:Thu 23 Apr 2020 05:12:03 PM EDT:.
(git-annex)lena:…-phantoms[master]bids_test3-20161011/AAHead_Scout_32ch-head-coil_MPR_cor
$> grep -A4 Orient *json
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00001.json: "ImageOrientationPatientDICOM": [
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00001.json- 0.987256,
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00001.json- -0.0511309,
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00001.json- 0.150699,
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00001.json- 0.159073,
--
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00002.json: "ImageOrientationPatientDICOM": [
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00002.json- 0.991633,
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00002.json- -0.0203033,
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00002.json- 0.127482,
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00002.json- 0.125499,
--
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00003.json: "ImageOrientationPatientDICOM": [
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00003.json- 0.990337,
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00003.json- -0.0649062,
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00003.json- 0.122559,
__AAHead_Scout_32ch-head-coil_20161011084552_4_i00003.json- 0.138123,
|
|
FWIW: woohoo -- based on checksum, that test scout we have is from http://datasets.datalad.org/?dir=/dicoms/dartmouth-phantoms/bids_test4-20161014/phantom-1/anat-scout_ses-localizer , for which we also have e.g. http://datasets.datalad.org/?dir=/dicoms/dartmouth-phantoms/bids_test4-20161014/phantom-1/anat-scout_ses-localizer_MPR_cor which has 3 dicoms which are similarly of the different orientations. So you might use those (instead of the ones I recommended) for consistency. ... those 3 files gzip into 100k, so in principle we could have carried their copy, but I would prefer not to as long as we can fetch them. |
bpinsard
force-pushed
the
multiple_converted_file_sliceorientationpatient
branch
from
eb60770 to
418601f
Compare
heudiconv/convert.py
Outdated
| ))) | ||
|
|
||
| is_multiorient = len(iops) > 1 | ||
|
|
There was a problem hiding this comment.
forgot to call update_mutliorient_name below?
would need also a unittest of that function (seems should be doable without files) and ideally some test files to ensure that use here works out
|
had a look and left "critical" observation(s). Will convert to draft for now - please undraft/ping whenever it is ready for re-review. |
Here is a PR to allow scans including variable slicing, such as localizer, to be labelled with
acq-<orient>instead of the suffix being indexed (default).As the different slicing still share a same SeriesID and are split by dcm2niix, I think it can only be done within heudiconv. Correct me if I am wrong.
Another case that doesn't seem to be yet managed is when data are exported with split channels, I might work on a separate PR unless someone feels like it.